Conversation
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #35356941211 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-18 14:51:18 UTC |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ab3b0eb to
4534a7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab3b0ebbe1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Normal liveness events can write an unknown leak tag. A long reference chain can also exceed the fixed JFR buffer and damage memory.
🤖 Datadog Autotest · Commit ab3b0eb · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
4534a7f to
71320e1
Compare
The null-pointer store is the deliberate never-returning crash, but clang scan-build reports a null dereference. Under __clang_analyzer__, replace it with __builtin_trap() - the analyzer understands the trap as a halt and explores no false path past it; the real build keeps the store.
Introduces the ReferenceChainEvent/ReferenceChainAbandonedEvent payloads (event.h), their JFR metadata (jfrMetadata.*), and the FlightRecorder emission paths that serialize chain events into JFR recording buffers, including the constant-pool handling for per-hop edge labels. Emission is pull-style: profiler.cpp snapshots events and hands them to FlightRecorder; this layer does not depend on the tracker itself.
71320e1 to
80173ad
Compare
Review findings on the JFR plumbing layer: - MAX_REFERENCE_CHAIN_EVENT_HOPS was a fixed 4096, permitting a ~438 KB worst-case event (near-limit edge labels) against a ~61 KB recording buffer - the reservation flushed first but the margin underflowed, so the write ran past the buffer (debug assert, release corruption). The cap is now derived from RECORDING_BUFFER_LIMIT minus the event's fixed fields, divided by the per-hop worst case, so a full-cap event always fits. - Truncation dropped ALL edge labels: the label count was gated on _edges.size() == emitted_size, which only holds for untruncated chains. Labels align with the chain's leaf-first element order, so truncation now emits the first emitted_size labels and loses only the root-side ones. - ObjectLivenessEvent::leak_tag is default-initialized to 0 so any construction path that forgets to set it serializes a defined untagged value (flush_table() overwrites it from the entry, which track() zeroes at insert). Moves the JFR round-trip and arguments parsing unit tests into this layer (they test exactly this code), rewrites the round-trip test to construct events directly instead of through the tracker, and adds byte-level boundary tests: oversize-chain truncation with label preservation, the size-prefix invariant, and the default leak tag.
d51dfb8 to
8b7de9c
Compare
rkennke
left a comment
There was a problem hiding this comment.
Nice change. I did a manual review, see below some findings.
| char *eq = strchr(cursor, '='); | ||
| if (eq) { | ||
| *(eq++) = 0; | ||
| // Floor every sub-option at the parse boundary rather than |
There was a problem hiding this comment.
That comment is way to verbose to justify why the values need to be floored and ceiling'ed. Either make them very concise or remove it altogether, it's kinda obvious that it's not a good idea to allow values outside the reasonable bounds here.
There was a problem hiding this comment.
Compressed to four lines: the hops negative-wrap case, the negative-budget collapse, the unbounded-values-into-loop-bounds case, and the one-boundary rationale. The details live in the constant comments in arguments.h now, stated once.
| const int DEFAULT_WALL_THREADS_PER_TICK = 16; | ||
| const int DEFAULT_JSTACKDEPTH = 2048; | ||
|
|
||
| // Every constant below is a provisional default pending empirical |
There was a problem hiding this comment.
Similar problem in this file: way too much comments explaining the same fact over and over again. Make it short and concise once, and drop all the other repetitions.
There was a problem hiding this comment.
Done - the block now states the provisional-default fact once up front, and each constant keeps only a note that says something the name does not.
| bool _record_heap_usage; | ||
| bool _gc_generations; | ||
| // Reference-chain tracking (PROF-15341 - see | ||
| // doc/architecture/LiveHeapReferenceChains-ImplementationPlan.md and |
There was a problem hiding this comment.
I don't think those docs exist in this PR, and they shouldn't, but the reference to them also needs to go. Also, I don't think we want Jira-ticket references in our code.
There was a problem hiding this comment.
Plan-document references are gone, and so are the Jira references in code (also swept the same pattern out of the upper layers of the stack).
| // otherwise; ReferenceChainTracker::fillHopEdgeLabels). Empty for events | ||
| // built before the edge-label change or when label resolution is | ||
| // unavailable (partial mock environments). | ||
| std::vector<std::string> _edges; |
There was a problem hiding this comment.
If _chain and _edges need to go hand-in-hand, then maybe make it a single vector of struct {u32; std::string } instead?
There was a problem hiding this comment.
Done - ReferenceChainEvent now carries a single std::vector<ReferenceChainHop> ({u32 klass_id; std::string edge_label;}), and the JFR writer derives both the chain array and the labels array from it. Labels are all-or-none per event (canary events carry none), so the writer treats any empty label in the emitted range as "no labels" - same bytes as before.
| }; | ||
|
|
||
| // Search-level abandonment signal (design doc's Termination section: | ||
| // "explicit reporting of abandoned searches ... no silent truncation"). |
There was a problem hiding this comment.
again, don't reference a non-existing design doc.
There was a problem hiding this comment.
Removed; the abandoned-event comment now states the rationale without citing the document.
| // documented in fillJavaMethodInfo below), the paired out-param size is just | ||
| // as likely to be corrupted, and an implausible size should be rejected | ||
| // before it is trusted to compute a byte range. | ||
| // JVM spec SS4.7.3 caps a method's bytecode (code_length) at 65535 bytes (u2). |
There was a problem hiding this comment.
This change seems unrelated?
There was a problem hiding this comment.
Reverted - it was an accuracy nitpick on a pre-existing comment and does not belong in this PR.
| // out-of-range index. | ||
| // | ||
| // STACK_LOCAL (24) and JNI_LOCAL (25) are labeled "first_observed_via:..." | ||
| // rather than plain "stack_local"/"jni_local" (design doc's "Honest labeling |
There was a problem hiding this comment.
stale referencs to design doc, again
There was a problem hiding this comment.
Removed the design-doc reference; kept the mechanism explanation (first_observed_via labels overstate what a popped frame proves).
Uncommitted plan documents, rotted .cpp:NNN line references, and a nonexistent j9WallClock.cpp path replaced with symbol references that stay valid as the code moves.
rkennke
left a comment
There was a problem hiding this comment.
🗿 🤖 🔴
Sphinx Review found 1 critical/high severity finding(s) that must be addressed.
| // a count of 0 means label collection never ran for this event. | ||
| buf->putVar32(edge_count); | ||
| for (u32 i = 0; i < edge_count; i++) { | ||
| buf->putUtf8(edge_labels[i]); |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — HIGH] recordReferenceChain() reserves MAX_REFERENCE_CHAIN_EDGE_LABEL (96) + 6 bytes per edge label, but writes labels with putUtf8(const char*), which truncates at MAX_STRING_LENGTH (8191). Any label longer than 96 bytes (a real Java field name easily can be) under-reserves by up to ~8 KB per hop and writes past the recording buffer - Buffer::put()'s only guard is an assert() that is compiled out in release builds. This is exactly the 'do not trust an upstream cap' argument the function's own comment makes for the chain length, but it is not applied to the labels.
Suggestion: Clamp each label's length to MAX_REFERENCE_CHAIN_EDGE_LABEL at the write site by using the two-argument putUtf8(v, len) overload, so the serializer's worst case matches its own reservation regardless of what the producer hands it.
| buf->putUtf8(edge_labels[i]); | |
| buf->putUtf8(edge_labels[i], (u32)(strlen(edge_labels[i]) < MAX_REFERENCE_CHAIN_EDGE_LABEL ? strlen(edge_labels[i]) : MAX_REFERENCE_CHAIN_EDGE_LABEL)); |
8b7de9c to
e3b3f66
Compare
- ReferenceChainEvent carries one vector of ReferenceChainHop (klass id + retention-edge label) instead of two parallel vectors - Compress the sub-option floor/ceiling rationale and the provisional default constant comments to one concise statement each - Drop design-doc and Jira references from code comments; revert the unrelated LineNumberTable comment rewrite
|
🔗 Commit SHA: b7bdec2 | Docs | View more details | Give us feedback! |
What does this PR do?:
Adds the JFR plumbing for reference-chain events: the
ReferenceChainEvent/ReferenceChainAbandonedEventpayloads (event.h), their dynamic JFR metadata and constant-pool handling (jfrMetadata.*), the FlightRecorder emission paths that serialize chain events and per-hop edge labels into recording buffers (flightRecorder.*), and thereferencechainsArgumentsblock (arguments.*) that configures the feature.Motivation:
Part 2 of the stacked series for reference chains (PROF-15341). Emission is pull-style — the later tracker layer snapshots events and hands them to FlightRecorder — so this layer is independently reviewable and compiles/links without the tracker.
Additional Notes:
Stacked series for PROF-15341. Merge order: #804 (scan-build crashNow fix) -> this PR -> #797 -> #798 -> #799 -> #800 -> #801 -> #802. #795 (UBSan ELF fix) is already merged. Until #804 lands, this PR's diff additionally shows the small faultInjection.cpp change from #804.
How to test the change?:
The JFR round-trip unit tests for chain events land in the gtest PR later in this stack;
buildDebugcompiles and links this layer standalone.For Datadog employees: